-
Notifications
You must be signed in to change notification settings - Fork 58
Add searchable record and search 🔍 #231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
0fe27d4 to
c710340
Compare
Records are added with simhashed tokens for later searching
| } | ||
|
|
||
| async onsearchablerecordput(req) { | ||
| if (!req.target || !this.searchableRecords) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (!req.target || !this.searchableRecords) return | |
| if (!req.target || !this.searchableRecords) return |
no req.reply(null) before return - intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, was intentional based on approach i saw on the other functions. req.reply(null) was added to onsearch so when we run https://github.com/holepunchto/hyperdht/pull/231/changes#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R192 there's no issues
Any downside?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no sure but if there's no req.reply called I'd have thought it could lead to a leak maybe, underlying stream wouldn't be ended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked with @mafintosh , all good since same in the other functions.
Records are added with simhashed tokens for later searching